Conversation
Convert .github/CONTRIBUTING.md and docs/contributing/guidelines.md from RST to Markdown, using MyST-parser (added in cherrypy#835) to render them. In addition, fix the "Also" section linking the rendered guidelines page back to itself: exclude it from the include instead of just rewording it, so it only shows up when reading CONTRIBUTING.md directly on GitHub. And drop CONTRIBUTING.md's opening pointer to the root README, which sends readers to a file with little relevant additional information for contributors. Also update one link in changelog-fragments.d/README.rst that broke once that file got pulled into the new Markdown page.
Documentation build overview
3 files changed± history/index.html± contributing/guidelines/index.html± pkg/cheroot.server/index.html |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #842 +/- ##
==========================================
+ Coverage 78.16% 78.20% +0.04%
==========================================
Files 41 41
Lines 4791 4791
Branches 548 548
==========================================
+ Hits 3745 3747 +2
+ Misses 906 905 -1
+ Partials 140 139 -1 |
|
@julianz- the CI is failing under Python 3.13+ due to a new validation check in coveragepy. Could you look into cherry-picking re-actors/alls-green@8de05dc in a standalone PR to fix the CI overall? |
| @@ -111,6 +112,3 @@ File :file:`docs/changelog-fragments.d/359.bugfix.rst`: | |||
|
|
|||
| See :file:`towncrier.toml` for all available categories | |||
| (``tool.towncrier.type``). | |||
|
|
|||
| .. _Towncrier philosophy: | |||
There was a problem hiding this comment.
Why did you do this? I actually intentionally prefer detached references everywhere so that they are clearly labeled and can be reused.
There was a problem hiding this comment.
Oh, the commit message says that one link broke. Did you mean this one?
@julianz- pro tip — you can leave additional notes right in the code review diff, as inline threads, like I do here. This helps enrich the context and saves me from having to guess.
There was a problem hiding this comment.
Sorry I made you hunt for the explanation. Will try to add comments more proactively as you suggest - thanks for the tip! Yes, this is the link that broke - the Towncrier philosophy reference and its target.
I originally thought to retain as an RST file in order to keep the changes smaller and more manageable. I see you suggested changing it below though, so maybe I will do that after all. The issue with it as an RST file, is that I had to pull it into the new guidelines.md page through an {eval-rst} block wrapping the old include directive. This generates ERROR: Unknown target name: "towncrier philosophy"with detached refs no matter where I place them presumably because of the eval-rst embedding.
| updating to the new version of the software will affect | ||
| end-users. This is why we enforce collection of the change | ||
| fragment files in pull requests as per `Towncrier philosophy`_. | ||
| fragment files in pull requests as per `Towncrier philosophy |
There was a problem hiding this comment.
Would the link still break if you keep put it after this paragraph? 🤔
There was a problem hiding this comment.
Yes tried that and it still breaks alas!
| :end-before: "## Also" | ||
| ``` | ||
|
|
||
| ## First-time setup |
There was a problem hiding this comment.
This sort of gives me vibes of a "this is how you use a computer". Wonder if this is useful at all or should link to something external. Although, I realize that a format conversion PR is not a place to discuss this. So this is just a side note for the future.
There was a problem hiding this comment.
Agreed - the noob vibes would be less grating if this were a lot shorter or pointed elsewhere as you suggest!
There was a problem hiding this comment.
@julianz- what if we updated this to be a README.md too? Would it be helpful or do you think this is out of the scope here?
There was a problem hiding this comment.
See above comment. I think maybe we should do this in order to get the more complete conversion over and done with.
webknjaz
left a comment
There was a problem hiding this comment.
@julianz- I'll let you decide whether you want to extend the scope a little or merge this as is.
In any case, I'd like a follow-up to reconfigure Towncrier to start using MD for the fragment files to make people's lives easier. This will require some amount of integration work, though. Here's a few places to pick up a few cool tricks from: https://github.com/jazzband/pip-tools/pull/2203/changes#diff-e010bb4b0f729415605939bacb0af8800a91a06d404da48b7e3fac5a74cddc90 / https://github.com/jazzband/pip-tools/pull/2343/changes#diff-e010bb4b0f729415605939bacb0af8800a91a06d404da48b7e3fac5a74cddc90R19
This removes the eval-rst workaround needed to embed it in guidelines.md, which was also the source of the detached-reference bug found in review. Also updates .gitignore, which only allowlisted README.rst by name and would have silently excluded the new file from tracking.
| @@ -1,13 +1,11 @@ | |||
| .. _Adding change notes with your PRs: | |||
| (adding change notes with your prs)= | |||
There was a problem hiding this comment.
Is this anchor used anywhere? We'll need to avoid using MyST-specific syntax in a few files that are also special-cased and rendered by GitHub itself. It uses GFM and anything MyST would show up broken in the render.
Examples include files like CONTRIBUTING.md and similar that are linked and embedded into the GH UI. So we both render them on GH and in the docs. We may want to split them into parts by using inclusions and keeping MyST syntax on the Sphinx side while only using what GFM allows in the shared parts of the docs. pip-tools' examples solved this pretty well.
Another example would be README files in any folders but especially next to the changelog fragments — just take a look: https://github.com/cherrypy/cheroot/tree/main/docs/changelog-fragments.d#readme — the context is right where people have expected to add these files. So I consider it important that this should render well. I suggest you to preview how this file renders in your fork, not only in the Sphinx build but also on GH, on the branch view.
| ## Examples for adding change log entries to your Pull Requests | ||
|
|
||
| File :file:`docs/changelog-fragments.d/77.doc.rst`: | ||
| File {file}`docs/changelog-fragments.d/77.doc.rst`: |
There was a problem hiding this comment.
For the same reason as above, this needs to drop the file role, I think...
|
|
||
| See :file:`towncrier.toml` for all available categories | ||
| (``tool.towncrier.type``). | ||
| ```{tip} |
There was a problem hiding this comment.
This is a MyST construct and GFM will not like it. In pip-tools, I've integrated some additional plugin or something for it to also support GFM-native admonitions that use Markdown blockquotes and start with > [!tip]
GitHub renders this file directly as the folder's own README, using
plain GFM, not Sphinx/MyST. Drop the MyST-only anchor, {file} role,
and {tip} directive that would have rendered as broken literal text
there: the anchor moves to guidelines.md, {file} becomes plain
backtick code, and {tip} becomes GitHub's native `> [!TIP]` syntax
(needs the new "alert" MyST extension to still render properly in
Sphinx). Also fixes the pre-commit hook that only allowlisted
README.rst and not README.md.
9f0240d to
8f8dc64
Compare
| affected issues when rendering the news file. | ||
| If you wish to sign your change, feel free to add ``-- by | ||
| :user:`github-username``` at the end (replace ``github-username`` | ||
| If you wish to sign your change, feel free to add `` -- by :user:`github-username` `` at the end (replace `github-username` |
There was a problem hiding this comment.
Will this work if we keep the line wrapped?
| If you wish to sign your change, feel free to add `` -- by :user:`github-username` `` at the end (replace `github-username` | |
| If you wish to sign your change, feel free to add `` -- | |
| by :user:`github-username` `` at the end (replace `github-username` |
| updating to the new version of the software will affect | ||
| end-users. This is why we enforce collection of the change | ||
| fragment files in pull requests as per `Towncrier philosophy`_. | ||
| fragment files in pull requests as per [Towncrier philosophy](https://towncrier.readthedocs.io/en/stable/#philosophy). |
There was a problem hiding this comment.
Can we keep ref this detached?
| fragment files in pull requests as per [Towncrier philosophy](https://towncrier.readthedocs.io/en/stable/#philosophy). | |
| fragment files in pull requests as per [Towncrier philosophy]. |
| https://towncrier.readthedocs.io/en/stable/#philosophy | ||
| > [!TIP] | ||
| > See `towncrier.toml` for all available categories | ||
| > (`tool.towncrier.type`). |
There was a problem hiding this comment.
| > (`tool.towncrier.type`). | |
| > (`tool.towncrier.type`). | |
| [Towncrier philosophy]: https://towncrier.rtfd.io/en/stable/#philosophy |
| Converted the contributing documentation (``.github/CONTRIBUTING.rst``, | ||
| ``docs/contributing/guidelines.rst`` and | ||
| ``docs/changelog-fragments.d/README.rst``) to Markdown, fixed a |
There was a problem hiding this comment.
I think we don't really need to mention the implementation details but could link the actual docs using RST refs so they'd be linked from the change log and be clickable..
|
@julianz- I think we should fix #842 (comment) first because the list of red jobs makes it really easy to miss that one failing RTD job that is actually related to the last update in this PR.. Also, I'm wondering what's different between the docs builds in GHA that makes one fail but the other one pass 🤔 |
|
@webknjaz I am a bit confused about which job is failing because of a coveragepy failure? Can you give me a link? |
|
Here: https://github.com/cherrypy/cheroot/actions/runs/36082117762/job/107906481438?pr=842#step:16:110 I posted the exact commit with the patch needed in the comment above. You just need to cherry-pick it into a separate PR. @julianz- how exactly are you checked what fails in PRs? When I scroll down to the checks widget in the PR, there's 33 failing checks (some of them are gray canceled jobs, some red, 3.15, 3.9, 3.8..). Could you post a screenshot of where you're looking at? |
I was ignoring the failed 3.15 tests as these chronically fail (in part because of issues I attempted to address in another PR I was going to open once #820 landed). I saw one failure in 3.14 but I ignored that too as you mentioned 3.13+ which I read as probably meaning 3.13 specifically. Anyway, I see the issue now that you have pointed me to a CI run with the error in question. |
myst-parser was unconstrained, so GitHub Actions and ReadTheDocs each resolved "latest" independently and landed on different versions -- GHA got 5.1.0, ReadTheDocs got 4.0.1, which predates the "alert" extension the last commit started relying on and broke ReadTheDocs's build as a result. The matching lockfile for this environment doesn't list myst-parser at all, so it was never constraining it either way; this one-line requirement is the complete fix on its own.
|
|
||
| furo | ||
| myst-parser[linkify] # Markdown documents support w/ in-text link detector | ||
| myst-parser[linkify]>=5.1.0 # Markdown documents support w/ in-text link detector; >=5.1.0 for the `alert` extension |
There was a problem hiding this comment.
|
|
||
| furo | ||
| myst-parser[linkify] # Markdown documents support w/ in-text link detector | ||
| myst-parser[linkify]>=5.1.0 # Markdown documents support w/ in-text link detector; >=5.1.0 for the `alert` extension |
There was a problem hiding this comment.
Let's make this sparse like other entries — I use the convention of spaced version specs for the sake of readability in hand-crafted files:
| myst-parser[linkify]>=5.1.0 # Markdown documents support w/ in-text link detector; >=5.1.0 for the `alert` extension | |
| myst-parser [linkify] >= 5.1.0 # Markdown documents support w/ in-text link detector; >=5.1.0 for the `alert` extension |
@julianz- additionally, there's a few constraint txt files (some are symlinks) in the same dir (https://github.com/cherrypy/cheroot/tree/main/dependencies) — they need to be updated using the same command and Python runtime as their headers say.
Per webknjaz's review suggestions: use a Markdown reference-style link (detached, defined at the bottom of the file) instead of an inline link, matching the project's preference for detached references elsewhere. Verified the reference resolves correctly both in the Sphinx/MyST build and in GitHub's own GFM rendering.
Per webknjaz's review suggestion: match the spacing convention used by the other hand-crafted entries in this file (e.g. Sphinx >= 1.8.2, jaraco.packaging >= 9) for readability.

What kind of change does this PR introduce?
What do these changes do?
Converts .github/CONTRIBUTING.md and docs/contributing/guidelines.md from RST to Markdown, using MyST-parser (added in Add MyST-Parser and link community files in docs sidebar #835) to render them.
Fixes the "Also" section so that it doesn't refer back to itself in the rendered version - it only shows up when reading CONTRIBUTING.md directly on GitHub.
Drops CONTRIBUTING.md's opening pointer to the root README, which sends readers to a file with little relevant additional information for contributors.
Updates one link in changelog-fragments.d/README.rst that broke once that file got pulled into the new Markdown page.
Are there changes in behavior for the user?
None — this only affects contributors reading the docs, not end users
Is it a substantial burden for the maintainers to support this?
No. It's a one-time format migration. Worth noting though:
changelog-fragments.d/README.rstremains RST, embedded viaeval-rstinto an otherwise-Markdown page — a minor format seam, not a burden.
Related issue number
Discussed with @webknjaz in chat; not tied to a filed issue.
Checklist
and description in grammatically correct, complete sentences
docs/) and inline docstrings reflect the changes(see [good commit messages])
(can be done once the PR number is known; alternatively, you can
use a related issue number if one exists)
[code of conduct][CoC]